-
Notifications
You must be signed in to change notification settings - Fork 523
Matter Switch: Add StatelessStep capability support for SwitchLevel and ColorTemperature #2677
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Duplicate profile check: Warning - duplicate profiles detected. |
|
Invitation URL: |
Test Results 71 files 481 suites 0s ⏱️ For more details on these errors, see this check. Results for commit 9bdd8d6. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 9bdd8d6 |
drivers/SmartThings/matter-switch/src/switch_handlers/capability_handlers.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/switch_handlers/capability_handlers.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/capabilities/statelessColorTemperatureStep.yml
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/switch_handlers/capability_handlers.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/switch_handlers/capability_handlers.lua
Show resolved
Hide resolved
80a5c0a to
00f1bf4
Compare
nickolas-deboom
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approved earlier but noticed there are some print statements from debugging I assume
|
@nickolas-deboom thanks for the catch, got those removed |
nickolas-deboom
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Looks like there is are test failures but I ran the tests locally and they pass, so they might need to be re-run in CI
drivers/SmartThings/matter-switch/profiles/light-color-level.yml
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/test/test_stateless_step.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/switch_handlers/capability_handlers.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/switch_handlers/capability_handlers.lua
Show resolved
Hide resolved
| local endpoint_id = device:component_to_endpoint(cmd.component) | ||
| -- before the Matter 1.3 lua libs update (HUB FW 55), there was no ColorControl StepModeEnum type defined | ||
| local step_mode = step_percent_change > 0 and (clusters.ColorControl.types.StepModeEnum and clusters.ColorControl.types.StepModeEnum.DOWN or 3) or (clusters.ColorControl.types.StepModeEnum and clusters.ColorControl.types.StepModeEnum.UP or 1) | ||
| local min_mireds = switch_utils.get_field_for_endpoint(device, fields.COLOR_TEMP_BOUND_RECEIVED_MIRED..fields.COLOR_TEMP_MIN, endpoint_id) or fields.COLOR_TEMPERATURE_MIRED_MIN -- default min mireds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be correct but just want to confirm, this should be COLOR_TEMP_MIN here and not COLOR_TEMP_MAX? The min mired value is the max kelvin value and vice versa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thing to check, but pretty sure this is OK. See this logic in the bounds handler:
switch_utils.set_field_for_endpoint(device, fields.COLOR_TEMP_BOUND_RECEIVED_KELVIN..minOrMax, ib.endpoint_id, temp_in_kelvin)
-- the minimum color temp in kelvin corresponds to the maximum temp in mireds
if minOrMax == fields.COLOR_TEMP_MIN then
switch_utils.set_field_for_endpoint(device, fields.COLOR_TEMP_BOUND_RECEIVED_MIRED..fields.COLOR_TEMP_MAX, ib.endpoint_id, temp_in_mired)
else
switch_utils.set_field_for_endpoint(device, fields.COLOR_TEMP_BOUND_RECEIVED_MIRED..fields.COLOR_TEMP_MIN, ib.endpoint_id, temp_in_mired)
end
so we set fields.COLOR_TEMP_BOUND_RECEIVED_MIRED..fields.COLOR_TEMP_MIN as the min value for mireds, and fields.COLOR_TEMP_BOUND_RECEIVED_KELVIN..fields.COLOR_TEMP_MIN as the min value for kelvin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes me head hurt so if you've tested and confirmed the right fields are getting set correctly that's good enough for me :)
| local endpoint_id = device:component_to_endpoint(cmd.component) | ||
| local step_mode = step_size > 0 and clusters.LevelControl.types.StepMode.UP or clusters.LevelControl.types.StepMode.DOWN | ||
| print("level:", step_size) | ||
| device:send(clusters.LevelControl.server.commands.Step(device, endpoint_id, step_mode, math.abs(step_size), fields.TRANSITION_TIME, fields.OPTIONS_MASK, fields.OPTIONS_OVERRIDE)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tpmanley @hcarter-775 What about using StepWithOnOff to turn on the light while it's off state?
Do we plan to support the level change during the 'off' state?
and Does transition(0) is best choice for the smooth transition? I wonder this is the best value from the test with real devices.
Now, all have '0' value.
lua_libs\st\matter\defaults\colorControl.lua
local TRANSITION_TIME = 0 --1/10ths of a second
lua_libs\st\matter\defaults\colorTemperature.lua
local TRANSITION_TIME = 0 --1/10ths of a second
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It really depends on the scenario, using the normal Step and a transition time of 0 is the most sensible default.
I have been playing with all those parameters for months with my custom driver for Matter lights and multiple buttons (including the new BILRESA scroll) so I will share the experience in case it is useful.
StepWithOnOff is great for dimmers that do not have on/off dedicated buttons since otherwise you would have no way to control the on/off state. It has two main drawbacks though:
- Cannot set the lights to the minimum brightness level. Most of the time you end up with the light turned off because it's virtually impossible to stop at the minimum. Or just impossible if the minimum is 1% and the steps are, let's say, 10%, it will go from 10% to off with the final step.
- If you are controlling multiple lights at the same time, they will all turn on or off. As a user, most of the time I do not want that behaviour and expect to control the brightness of the lights that are on, not turn on everything.
Transition time is a great feature of Matter that should have more presence in the capabilities. setLevel of switchLevel has it but not the commands to set colours or colour temperature. It is something requested time ago and the main reason for me to use a custom driver.
When it comes to stepping the brightness level, using transitions is tricky:
- Some lights flood the hub with brightness level reports while stepping, especially before Matter 1.4 made quiet reporting mandatory. That makes the hub sluggish for a moment and the user would feel the dimming is not responsive. With some lights a transition time of 1 (tenth of a second) the experience is fine, with others it's better to leave it at 0 since they already apply some default transition anyway.
- If two step commands are sent too close in time and the first one did not end the transition yet, the first step is essentially killed. That's easy to experience with a BILRESA scroll since the steps can happen too fast as you scroll.
For added flexibility, the capability should have optional second and third command arguments for the transition time and the withOnOff behaviour. That would make the default 0 and normal Step if not provided. Maybe it is not late to add them being a proposed capability not yet used elsewhere.
While unrelated to the steps, adding the transition time argument to the commands to set colour and temperature would be great too, like in setLevel of switchLevel capability. It should not break anything since the default is 0 anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hcarter-775 Thank you for your detailed explanation.
It could be linked to the below conversion, so we need decide more beneficial UX for the user.
- 'turning on' before changing the level
- keep 'off' state and just change level silently.
- doing nothing while 'off' state.
and Regarding the 'transition time', what about adding the preference setting in edge driver for this?
We can't expect same result for different models, so we can give an option to the user. (default:'0')
Definitely, we need to support multi-language for the new setting menu.
cc: @tpmanley
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hcarter-775 Thank you for your detailed explanation
That was my comment actually, I hope I didn't create any confusion! I'm the developer of the Matter Playground custom driver and thought it was useful to share my feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mocelet Oh, sorry my mistake. Thank you for sharing your thought and experience.
| local max_mireds = switch_utils.get_field_for_endpoint(device, fields.COLOR_TEMP_BOUND_RECEIVED_MIRED..fields.COLOR_TEMP_MAX, endpoint_id) or fields.COLOR_TEMPERATURE_MIRED_MAX -- default max mireds | ||
| local step_size_in_mireds = st_utils.round((max_mireds - min_mireds) * (math.abs(step_percent_change)/100.0)) | ||
| print("percent:", step_size_in_mireds) | ||
| device:send(clusters.ColorControl.server.commands.StepColorTemperature(device, endpoint_id, step_mode, step_size_in_mireds, fields.TRANSITION_TIME, min_mireds, max_mireds, fields.OPTIONS_MASK, fields.OPTIONS_OVERRIDE)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hcarter-775 @tpmanley
Don't we need to turn on the light if the light is off state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@inasail We could do this, so it depends how we expect the capability to be used. In my testing, I had one button routine to turn the device on/off, and then I separately used the scroll functionality.
Since it doesn't make sense to turn the device off given the scroll functionality, I wonder if it makes as much sense to turn the device on with this capability?
I also wonder how nice this user experience would be, since the scrolling would change the color temperature the device is on while it turns it on. This seems like it may create an awkward experience. What do you think? cc: @tpmanley
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hcarter-775 I think we need to consider how often users will actually need to change the switch level and color temperature while the device is turned off. If we don't turn on the light. Users should always turn on by themselves first and rotate the switch. It needs double actions all the time.
But on the other hand, like you said, if there are multiple lights in a group where some are on and others are off(Even though controlling multiple devices via a knob switch isn't supported yet)and we may want to adjust the switchLevel and colorTemperatureLevel only for the lights that are already 'on' while keeping the others 'off', then it seems we shouldn't necessarily force them to turn on.
For that 'doing nothing' if the light is 'off' state could be another option. Let's think more.
cc: @tpmanley @greens @varzac
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For that 'doing nothing' if the light is 'off' state could be another option
Alternatively, just send the Step like it is now without checking the on/off state and let the light handle it. When the normal Step is received, a Matter light that is off will change the level/temperature without turning on.
Even though controlling multiple devices via a knob switch isn't supported yet
By the way, I use the knob with Rules API to control multiple devices and the popcorn effect using an Aeotec v3 hub is quite noticeable. Hopefully a native handler is on the way like for the absolute brightness level.
Since soon there will be no going back, please consider adding a couple extra arguments to the stateless step capability commands: an integer for the transition time and a boolean for the withOnOff behaviour. There is no one size fits all and, even if the app UI only offers a sensible default with no configuration, having the option to customize the behaviour with a rule is more than welcome.
Description of Change
Update all profiles to support statelessStep capabilities as needed. Include handlers for these step capabilities using the Matter-provided step commands.
This breaks apart the work found in #2669 so that the stateless capability support can be reviewed as an isolated unit.
Summary of Completed Tests
Unit tests added. Tested on-device.